Add mesa snapshot overlay #72

lumag · 2025-06-12T09:55:50Z

In order to evaluate upstream Mesa on the RB1 board, package the snapshot of the Mesa development tree. This package will go away once Mesa 25.2 gets released and enters Debian experimental or unstable.

lool · 2025-06-12T10:40:20Z

I gave this a cursory read to review the general approach, and it's as we discussed. I'm not super happy that we duplicate packaging in this repo and that we allow for calling random scripts, but that works in the short-term.

@basak-qcom Would you be able to do a full review? Or I can do one on Monday

github-actions · 2025-06-12T11:14:27Z

Test Results

2 files ±0 4 suites ±0 6m 20s ⏱️ +23s
15 tests ±0 15 ✅ ±0 0 💤 ±0 0 ❌ ±0
38 runs ±0 38 ✅ ±0 0 💤 ±0 0 ❌ ±0

Results for commit ba0c2a3. ± Comparison against base commit ca08110.

♻️ This comment has been updated with latest results.

github-actions · 2025-06-12T11:14:28Z

Test jobs for commit `e5f2734`

github-actions · 2025-06-12T12:48:02Z

Test jobs for commit `7ea28c1`

basak-qcom

[Comment only] Ideally we'd have a better mechanism to do all of this, such as packaging repositories in git that can derive from upstream and Debian packaging commits. But in the meantime to make progress we already have overlay-debs debdiffs as a compromise. Adding a generic script mechanism seems like a clean way of approaching this. Even though it's more creep towards the less ideal, I can't think of a better general approach.

[No change needed] We have a service dependency on gitlab.freedesktop.org being introduced here, whereas before we depended on Debian apt archives only I think. Again, given this is a stop-gap solution, I guess that's OK. Perhaps we should start noting these somewhere though.

[For discussion before landing] The debian/ directory and packaging inside gives me some concern. It's being copied from Debian experimental I think, and then being modified? Since our goal is to minimise any "delta" to upstream over time, ideally we'd store the diff rather than a copy. Notably this approach is the one we take with the overlay-debs debdiff mechanism, which is what this is building upon. I wouldn't do that for the entire upstream snapshot, but perhaps we can do that for the packaging, supplying a debdiff to apply over the source package once it has been constructed from the upstream git commit and the debian directory from Debian experimental? Otherwise, it took me some effort to fetch and generate a diff against Debian experimental to review, and then I'm finding it difficult to understand why you've changed some aspects of the packaging against Debian experimental. For someone else to understand that is key to allowing that difference to be eliminated in the future. What do you think about storing a diff against experimental instead, and then generating that dynamically as part of the script?

[Changes requested] Please could you link directly to the upstream PRs or commits that you are cherry-picking from the patch files themselves? For example there's dep3 which describes what metadata should go into a patch in debian/patches. In this case of squashing multiple commits together, I think that's fine in which case maybe just dump the headers at the top? I don't know of any tool that reads these; it's just really useful when rebasing in the future, so I'd like to make it a standard that the information is available. I think this should be in the source tree itself rather than just in commits.

scripts/build-deb.py

In order to build upstream mesa snapshot we need to manually prepare source directory. Add support for using the external script instead of just fetching the dsc from Debian. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

In order to evaluate upstream Mesa on the RB1 board, package the snapshot of the Mesa development tree. This package will go away once Mesa 25.2 gets released and enters Debian experimental or unstable. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

Import patchset from Mesa MR 35316, implementing 24-bit texture formats support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

lumag · 2025-06-17T13:31:34Z

@lool @basak-qcom reworked the PR

Corresponding packages were uploaded to OBS

github-actions · 2025-06-17T14:36:42Z

Test jobs for commit `53e17b8`

lool

Hey, thanks for implementing this, and sorry for the delay in reviewing on my side; I've left mostly minor comments but please could you:

change the Debian revision to 0qcom1
add some notes on how you created the debdiff, either in the commit message or in debian/changelog inside the debdiff
pass this new script through the shellcheck workflow (.github/workflows/static-checks.yml only runs against ci/*.sh).

Thanks!

lool · 2025-06-19T06:58:29Z

scripts/build-deb.py

@@ -59,8 +59,24 @@
        )
    print("✅ Checksum of original source package matched.")

-    # Unpack the source package
-    subprocess.run(['dpkg-source', '-x', dsc_file], cwd=temp_dir, check=True)
+    script = config.get('script')


I guess this could be named "source_download_script"

Maybe source_unpack_script? Since it replaces the dpkg-source -x call, not the .dsc file download.

No. It does more than just dpkg-source -x. As such, I'd prefer to keep it as is. I think it should be possible to build an overlay deb without original DSC at all.

lool · 2025-06-19T07:07:39Z

scripts/build-deb.py

+            script = os.path.abspath(os.path.join(config_dir, script))
+
+        env = os.environ.copy()
+        env['DSC_FILE'] = dsc_file


I'd prefer if we passed the whole yaml file to the script, but I know it's not fun to parse yaml from shell; that's actual actually why I wrote build-deb in python, but now we're extending it to call a shell script :)

I guess this is a temporary solution and we need to get rid of this debdiff juggling logic ASAP, but this will do for now.

lool · 2025-06-19T07:09:27Z

overlay-debs/mesa-snapshot/mesa-snapshot.sh

+tar xJf ${DEB_FILE}
+
+cat >> debian/changelog.tmp << EOF
+mesa (${version}-0) experimental; urgency=medium


would you mind changing the version to 0qcom1 so that all packages with qcom changes have a qcom version?

basak-qcom · 2025-06-20T14:18:31Z

I like the overall mechanism now - thanks! It's quite clean given the compromise we're already making - still based on a Debian dsc from the Debian snapshot archive, and the script doing the quite minimal upstream snapshot extraction before the debdiff is applied afterwards as normal for the packaging changes.

Next I looked at the packaging changes themselves. Here's a summary of all the changes I see to packaging against the base Debian 25.1.0-1 package:

Revert to LLVM 19 (LLVM 20 is not in trixie so this makes sense)
Build against librust-rustc-hash-2-dev, not librust-rustc-hash-dev (although both are in trixie?)
Drop packages
- libxatracker2
- libxatracker-dev
- libd3dadapter9-mesa
- libd3dadapter9-mesa-dev
Update libegl-mesa0.symbols with three new symbols
mesa-opencl-icd.install: drop some files
debian/patches/35316.patch: new patch ("freedreno: Add sampling support for RGB/BGR 24-bit component texture formats")
debian/patches/etnaviv-add-support-for-texelfetch.patch: drop (but it doesn't reverse-apply to this snapshot cleanly)
debian/patches/path_max.diff: tweak
debian/rules: various tweaks
debian/control regenerated from debian/control.in

With my Ubuntu background, I'm used to maintaining a delta against an upstream distribution, including upstream snapshots. In that context, there are usually a couple of concerns driven by the desire to converge back with upstream in the future:

When it comes time to drop the delta, will this cause an unwanted change in user behaviour?
When rebasing our delta onto a newer upstream version (because we can't or don't want to drop the delta yet), is each part of the delta appropriate to keep, does it need adjusting, or can we drop it without causing an unwanted change in user behaviour?

For this reason my default expectation is that the delta should be minimal and the reason for each part of the delta existing should be obvious or explained. In Ubuntu we have largely moved to using git to maintain the delta directly, so each part is a commit with an explanation. Here, with everything being squashed into the single debdiff, we don't have that ability yet, unfortunately. Nevertheless I can still apply my usual expectations, consider the debdiff and try to understand why each part is there.

How does my default expectation from Ubuntu map to this project? I'm not sure yet! With our "upstream first" philosophy, I think we are agreed that the delta needs to be kept at a minimum. I think the depth we need to spend on the details of the delta itself depends on what user stories we need to support with this package, together with our expectations on how long we will need to maintain this delta for. Is it possible that a mesa snapshot delta isn't short-lived because of another requirement that comes along in the future before the current requirement is upstreamed, for example?

Before we used git in Ubuntu, our convention was to explain the delta in debian/changelog including upon every rebase, and we still do that today. Since we can't use git, perhaps we could do the same here? For example:

mesa (25.2~2640-g8d13fc447e3-0qcom1) trixie; urgency=medium

  [ Dmitry Baryshkov ]
  * New upstream snapshot to evaluate upstream Mesa on the RB1 board. 
  * Backport to trixie:
    - Revert to building with LLVM 19 since 20 is not in trixie.
    - Drop packages: libxatracker2, libxatracker-dev,
      libd3dadapter9-mesa, libd3dadapter9-mesa-dev.
    - Build against librust-rustc-hash-2-dev, not librust-rustc-hash-dev
      [both are in trixie?].
    - Update libegl-mesa0.symbols.
    - d/mesa-opencl-icd.install: drop some files [does it fail to build without this?].
    - d/p/35316.patch: freedreno: Add sampling support for RGB/BGR
      24-bit component texture formats.
    - d/p/etnaviv-add-support-for-texelfetch.patch: drop [although it doesn't reverse apply exactly?]
    - debian/patches/path_max.diff: [tweak].
    - debian/rules: [tweak; multiple hunks here and the reasons for most aren't clear to me].
    - debian/control regenerated from debian/control.in.

 -- Robie Basak <robibasa@qti.qualcomm.com>  Fri, 20 Jun 2025 13:06:04 +0100

I've tweaked the version string to include qcom1 as Loic requested, and also tried to select something that should work for future updates smoothly.

Proposed next steps:

I'm open to taking a different approach, or in less detail, if that seems appropriate.
Otherwise, please could I have some help in explaining why we're making these changes to packaging by adjusting my changelog example above, replacing my lack of understanding annotated in the square brackets?

With help as above, I'd be happy to take over driving this to getting it landed if you like.

lumag · 2025-06-20T19:36:30Z

@basak-qcom too many words for a package that is going to be obsoleted in July or August. Could you please make your comments easier to comprehend?

Update commit ID to a recent one, which, among other changes, pulls in FP16 fixes in the freedreno driver. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

Opt-in and use the gallium-rusticl-enable-drivers in order to enable RustiCL drivers which can be enabled by default: asahi, freedreno and radeonsi. The rest of the drivers still need an explicit RUSTICL_ENABLE environment variable. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

github-actions · 2025-06-23T11:25:57Z

Test jobs for commit `ca86dfe`

Specify debian revision as -0qcom1 instead of just -0 in order to point out that it is a Qualcomm-specific build. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

github-actions · 2025-06-23T14:23:01Z

Test jobs for commit `ba0c2a3`

This fixes shellcheck "SC2086 (info): Double quote to prevent globbing and word splitting." Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

basak-qcom · 2025-06-25T13:35:55Z

@lumag I pushed 76162c8 directly to your branch to fix some shellcheck warnings. I'm confident that's unobjectionable. I trust that's appropriate process/etiquette here?

Three more changes to come: one remaining shellcheck issue, I'll add a README, and I'll rebase onto main in order to pick up the shellcheck CI changes. Please let me know if I should do this differently.

DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py here. Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

lumag · 2025-06-25T21:00:33Z

overlay-debs/mesa-snapshot/mesa-snapshot.sh

+# be assigned. Did you mean DEB_FILE?
+#
+# DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py
+# shellcheck disable=SC2153


NAK, don't add extra noise.

Just removing it will cause it to fail CI. Please clarify how you want me to fix it instead.

Then please fix the CI. Adding 5 lines of comments, including the tools output, just to make it ignore a variable name in the script is definitely an overkill.

we can probably do this on one line, something like:

# shellcheck disable=SC2153 # DSC_FILE is guaranteed to be defined by build-deb.py

or we could set DSC_FILE if unset at the top of the file, with a comment saying it should be set by build-dep.py

See my general comment on lint tools. I think this is fine as-is. I don't think it'd be productive to spend time iterating to find something that the linter will accept. It's wrong, so we can say it's wrong and move on. For the sake of a comment, I really think this is fine.

Sorry, I hadn't seen lool's comment. If your suggestion is acceptable then sure - it's not worth the discussion.

lumag · 2025-06-25T21:01:17Z

overlay-debs/mesa-snapshot/README.md

+        can be enabled by default: asahi, freedreno and radeonsi.
+      + Disable gallium-xa and gallium-opencl=icd.
+      + No longer need to remove mme_{fermi,tu104}_sim_hw_test
+    - debian/control regenerated from debian/control.in.


Just add this to the script so that it goes to the actual changelog.

I agree that would be better, but you are generating the changelog - parameterised - from your script. I didn't put this directly in there since it'll be wrong as soon as the parameters change. I can either keep it in the README.md as-is, or remove the changelog generation from the script and put a changelog entry via the debdiff instead. Which do you prefer?

Exactly because of that. If you want a detailed changelog entry, please generate it from the script. This way apt-listchanges will show it to users, making sure that they see any actual changes to the snapshot packaging.

I'd be happy to generate it automatically from a script, but that would mean that we need separate commits with the same level of detail, and we can't do that right now because we're using flat debdiffs. This is the compromise we're making. If we want to land this in a practical timescale, I can keep the README.md, or remove the changelog generation and move the entire changelog statically into the debdiff. Which do you want?

lumag · 2025-06-26T10:34:59Z

overlay-debs/mesa-snapshot/mesa-snapshot.sh


 rm -rf mesa
 git clone --depth 1 https://gitlab.freedesktop.org/mesa/mesa

 cd mesa
-git fetch --depth 1 origin ${COMMIT##origin/}
+git fetch --depth 1 origin "${COMMIT##origin/}"


We don't need to quote all the variables. Only the variables that might have whitespaces or might be empty need to be quoted.

Yup, shellcheck doesn't know that, so it asks to quote everything by default, but we can add an override to tell it to ignore this particular line; I think it's cheaper to add the double quotes personally, even if it's a bit more verbose

See my general comment on lint tools and CI. The compromise is that even though it's not necessary, it's not wrong, so this is fine as-is. That way we can have a linter and CI, which is better for our culture overall.

basak-qcom

I think we need to talk about CI and lint tools.

The culture I think we need to be heading towards is:

Use linting in CI to give immediate contributor feedback and achieve a baseline of quality, both for our repositories and to set an example across the company.
Lint tools are indeed dumb. They will make us do things that aren't necessary. That's the cost of achieving the previous point, but the benefit is worth the cost.
When the lint tool is wrong, a simple override should be enough, using whatever mechanism the lint tooling provides. Best practice is to explain how the lint tool is wrong in a comment, to avoid suppressing the lint tooling when not appropriate ensuring that this can be reviewed effectively.
If the linter is wrong, just override it and move on. I don't think it's worth the effort of trying to figure out how to work around the linter so that it won't alert.
Fixing the linter is out of scope. Perfect is the enemy of the good. We have better uses for our time. Just using the linter gives us enough bang for the buck.
This does mean apparently unnecessary noise, but the general best practice I'm used to is that it's worth the cost.

I'm surprised to see this being nitpicked here, and indeed this isn't something that's worth debating in a PR. The above should just be in a documented best practice document so that we do not endlessly debate this in PRs.

basak-qcom · 2025-06-26T15:19:58Z

overlay-debs/mesa-snapshot/README.md

+        can be enabled by default: asahi, freedreno and radeonsi.
+      + Disable gallium-xa and gallium-opencl=icd.
+      + No longer need to remove mme_{fermi,tu104}_sim_hw_test
+    - debian/control regenerated from debian/control.in.


I'd be happy to generate it automatically from a script, but that would mean that we need separate commits with the same level of detail, and we can't do that right now because we're using flat debdiffs. This is the compromise we're making. If we want to land this in a practical timescale, I can keep the README.md, or remove the changelog generation and move the entire changelog statically into the debdiff. Which do you want?

basak-qcom · 2025-06-26T15:20:50Z

overlay-debs/mesa-snapshot/mesa-snapshot.sh

+# be assigned. Did you mean DEB_FILE?
+#
+# DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py
+# shellcheck disable=SC2153


See my general comment on lint tools. I think this is fine as-is. I don't think it'd be productive to spend time iterating to find something that the linter will accept. It's wrong, so we can say it's wrong and move on. For the sake of a comment, I really think this is fine.

basak-qcom · 2025-06-26T15:21:31Z

overlay-debs/mesa-snapshot/mesa-snapshot.sh


 rm -rf mesa
 git clone --depth 1 https://gitlab.freedesktop.org/mesa/mesa

 cd mesa
-git fetch --depth 1 origin ${COMMIT##origin/}
+git fetch --depth 1 origin "${COMMIT##origin/}"


See my general comment on lint tools and CI. The compromise is that even though it's not necessary, it's not wrong, so this is fine as-is. That way we can have a linter and CI, which is better for our culture overall.

lumag force-pushed the mesa branch from f2e3ca8 to e5f2734 Compare June 12, 2025 10:16

lool requested a review from basak-qcom June 12, 2025 10:36

lumag force-pushed the mesa branch from e5f2734 to 7ea28c1 Compare June 12, 2025 11:47

basak-qcom requested changes Jun 16, 2025

View reviewed changes

scripts/build-deb.py Outdated Show resolved Hide resolved

lumag added 3 commits June 17, 2025 16:13

overlay-debs/mesa-snapshot: import patchset implementing RGB888 support

53e17b8

Import patchset from Mesa MR 35316, implementing 24-bit texture formats support. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

lumag force-pushed the mesa branch from 7ea28c1 to 53e17b8 Compare June 17, 2025 13:28

lool requested changes Jun 19, 2025

View reviewed changes

lumag added 2 commits June 23, 2025 13:26

overlay-debs/mesa-snapshot: pull in FP16 fixes

87c1132

Update commit ID to a recent one, which, among other changes, pulls in FP16 fixes in the freedreno driver. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

overlay-debs/mesa-snapshot: use qcom-specific version

ba0c2a3

Specify debian revision as -0qcom1 instead of just -0 in order to point out that it is a Qualcomm-specific build. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

mesa-snapshot.sh: quote shell variables

76162c8

This fixes shellcheck "SC2086 (info): Double quote to prevent globbing and word splitting." Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

basak-qcom added 2 commits June 25, 2025 14:51

mesa-snapshot.sh: ignore shellcheck false positive

801a273

DSC_FILE is guaranteed to be defined as part of the call API from build-deb.py here. Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

mesa-snapshot: add README.md delta description

37bd828

Signed-off-by: Robie Basak <robibasa@qti.qualcomm.com>

lumag commented Jun 25, 2025

View reviewed changes

lumag commented Jun 26, 2025

View reviewed changes

basak-qcom reviewed Jun 26, 2025

View reviewed changes

Add mesa snapshot overlay #72

Are you sure you want to change the base?

Add mesa snapshot overlay #72

Uh oh!

Conversation

lumag commented Jun 12, 2025

Uh oh!

lool commented Jun 12, 2025

Uh oh!

github-actions bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

github-actions bot commented Jun 12, 2025

Test jobs for commit e5f2734

Uh oh!

github-actions bot commented Jun 12, 2025

Test jobs for commit 7ea28c1

Uh oh!

basak-qcom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lumag commented Jun 17, 2025

Uh oh!

github-actions bot commented Jun 17, 2025

Test jobs for commit 53e17b8

Uh oh!

lool left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basak-qcom commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lumag commented Jun 20, 2025

Uh oh!

github-actions bot commented Jun 23, 2025

Test jobs for commit ca86dfe

Uh oh!

github-actions bot commented Jun 23, 2025

Test jobs for commit ba0c2a3

Uh oh!

basak-qcom commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 12, 2025 •

edited

Loading

Test jobs for commit `e5f2734`

Test jobs for commit `7ea28c1`

Test jobs for commit `53e17b8`

basak-qcom commented Jun 20, 2025 •

edited

Loading

Test jobs for commit `ca86dfe`

Test jobs for commit `ba0c2a3`